-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/stratified therm storage a1 #718
Conversation
Hi Marie! I am confused. Can you explain to me why you created a specific branch for this? If it is because you did a main decision, you could have merged this branch into the other branch (#711) for a seperate fix. |
Oh sorry, I understood this from the following sentence in #711 (comment):
I also thought that it would be useful to leave PR #711 as it is in case I or somebody else should decide to implement the thermal losses through |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MaGering!
So, I would like to keep the MVS so, that it allows that you two new parameters are not defined. I propose a solution. Please try to implement it, and I hope that you can then revert all the input files back to where they where. If not, I can take a look in January.
You can always work on this branch in the next week for your simulations.
What is the result regarding minimum=1
?
tests/benchmark_test_inputs/ABE_grid_PV_battery/csv_elements/storage_1.csv
Outdated
Show resolved
Hide resolved
Cool, thank you very much Martha! I will try to get the simulations running with your propositions. :)
Well that's a little complicated: It is actually this I tested several use cases and discovered, that the simulation would run without error if this AttributeError didn't exist. I would not have to specify a minimum capacity. I still couldn't figure out (from rtd etc.) why it is necessary. |
So it is a generic oemof-solph issue for the new attributes? Weird! I hate the idea of it but... Have you tried with |
For my use case at least it seems to be. However I can not estimate whether there is not a necessity of it or advantages in other use cases.
Yes, I have tried this and it works as well. But I don't like this either. I thought maybe to first find the purpose of the AttributeError. If there is a good reason I would go with the dirty hot fix. But otherwise I would propose to just do a warning instead. An information I got from Jann is, that the absolute losses exist independently from the installed capacity so they would exist even if there was no storage installed. However, this has not been confirmed when I tested it in separate use cases (one without thermal energy storage and one with a super expensive thermal energy storage. The super expensive one hasn't been installed and in both cases the heat demand was the same). I think I will address this issue after the holidays again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that it works. Will look at it in greater detail after the holidays.
Please create the pytest then to make sure this works (pytest for changed function in A1 + benchmark test).
6998795
to
8a5e528
Compare
Hi @MaGering! So, should I review this PR now? |
Hello Martha, I found that I've been wrong with the losses. They are comparatively small but they are being calculated as Jann said even if no storage is installed (because too expensive). So the Before merging this branch the question remains how we want to proceed with this to avoid hardcoding an investment minimum we actually don't want. An idea, @c-moeller had, is integrating Can you estimate the effort of doing this? |
I am also trying to understand the calculations in oemof.solph and see if there is another way to take out the losses and avoid the |
Hi @MaGering ! Could you maybe summarize the findings of our call yesterday here / your plans so that nothing gets lost? |
Sure! :) Since the problem of the I wrote an issue in oemof.thermal describing the problem. Now there are the following open tasks:
|
Do you also plan to integrade a benchmark test with the stratified termal storage in ...I am not sure how we can make sure that the generic storage in dev and in your PR has identical results, but to do it manually. You did that again, right? |
Yes, this is what I'm also working on right now besides the pytest of the changed function in A1, documenting the dirty fix in the readthedocs (bullet point from my comment see above) and documenting the changes in As for the benchmark test in both cases it is the generic storage component. In case of a stratified thermal storage only the two further thermal losses are passed with the generic storage component. So a thermal storage without the two thermal losses is just a generic storage. :) I think this would be rather a test in oemof.solph but I could try to write a benchmark test where I build a generic storage as it is now in dev and one with thermal losses that equal zero and compare their results. What do you think @smartie2076? So the benchmark tests to implement are:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MaGering I did not work though the whole PR jet (benchmark test missing). I still need to execute your code locally to make sure everything works.
Some remarks already now, including some RTD stuff. Please add the name of the pytests of functions to the original functions` "Notes" section, and also write debug messages.
@Bachibouzouk with this PR, there are two parameters THERM_LOSSES_REL
and THERM_LOSSES_ABS
added to the storage_xx.csv
. They are not to be used for the MVS within E-Land, and should also be set with a default value if we do use json input files. Therefore, they should also be added to #456 and/or #750.
@SabineHaas I did a pre-check of this PR now. If you had time, it would be great if you also checked it completely. The D1
tests are hard for me to read, so please check those.
docs/Model_Assumptions.rst
Outdated
@@ -63,6 +63,61 @@ In case of excessive excess energy, a warning is given that it seems to be cheap | |||
High excess energy can for example result into an optimized inverter capacity that is smaller than the peak generation of installed PV. | |||
This becomes unrealistic when the excess is very high. | |||
|
|||
Stratified thermal storage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to Energy storage
and descibe how the generic energy storage is used, and then stratified thermal storage
as a specific case of energy storages?
Outline of content:
Generic storages are defined with file x
and y
, and have subassets a, b, c. Self-discharge... efficiency of charge/discharge non-dependent on soc... rate at which battery can be (dis)charged relative to its capacity (crate)... battery lifetime based on years, not charge cycles... for now now KPI measuring the storage utilization (charge cycles, average charge ect).
Stratified storage uses two optional parameters: d, e. If those two are not included in the csv file (or 0), then a normal generic storage is simulated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generic storages are defined with file
x
andy
, and have subassets a, b, c. Self-discharge... efficiency of charge/discharge non-dependent on soc... rate at which battery can be (dis)charged relative to its capacity (crate)... battery lifetime based on years, not charge cycles... for now now KPI measuring the storage utilization (charge cycles, average charge ect).
I don't quite understand. Isn't this section described in Parameters and Definitions in CSVs/JSON
in the docs? I can list the subassets but do I need to describe them again or can I just reference to Parameters and Definitions in CSVs/JSON
? And should I actually add the fixed thermal losses to MVS_parameters.rst
as well or did you not want them in there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you don't use references to other rst files in rtd, or did you just not have a need for it until now?
Otherwise I would suggest this:
- Adding
sphinx.ext.autosectionlabel
to theextensions
indocs/config.py
- Adding a label to the subsection
storage_*.csv
indocs/MVS_parameters.rst
- Adding the following sentence to docs/Model_Assumptions.rst:
Generic storages are defined with file `energyStorage.csv` and `storage_*.csv` and have subassets, which are listed in :ref:`storage_*.csv`.
I've tested it locally and it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I've found a solution in commit ad4ef15
What do you think @smartie2076? Is it necessary to describe the generic storage in more detail?
tests/benchmark_test_inputs/Economic_KPI_C2_E2/csv_elements/storage_1.csv
Outdated
Show resolved
Hide resolved
Will do that :) @MaGering could you give me a hint after you have worked on all the comments? I think it makes sense that I wait until then. |
Sure, this makes sense! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really impressive work, especially the benchmark tests :D
You can add a mention of your benchmark tests here, summarized in one bullet point.
I reviewed the PR again, ran all tests locally and think that it can be merged. There is only the one issue that if you only change the tests/input/mvs_config.json
file, not the csvs, it is very likely that those will be overwritten in future PR.
There are also some warning messages that you could adress by changing the input file values of your input files of installedCap
from NaN to a valid value (Warning for multiple of your input files):
In file storage_optimize_without_fixed_thermal_losses.csv the parameter installedCap in column storage capacity is NaN. Please insert a value of 0 or int. For this simulation
the value is set to 0 automatically.
Next time with such a big PR you should clean it up afterwards (do git rebase --soft commitnumber
and redo your commits), as it is much easier to read. Honestly, I would probably have been quicker about it then as it just requires less time and concentration that way ;)
@@ -37,6 +38,10 @@ Here is a template for new release sections | |||
- Gather all missing MVS parameters and raise a single error listing all of them (#761) | |||
- Add `set_default_values` argument to the `B0.load_json` function to set default values of missing parameter which is listed in `KNOWN_EXTRA_PARAMETERS`(#761) | |||
- Add `flag_missing_values` argument to the `B0_load_json` function to allow switching between `MissingParameterWarning` and `MissingParameterError`(#761) | |||
- In `test_A1_csv_to_json.py` tests were added that check whether default values of `0` are set for `fixed_losses_relative` and `fixed_losses_absolute` in case the user does not pass these two parameters (#718) | |||
- In `test_D1_model_components.py` tests were added that check whether the `GenericStorage` parameter `investment.minimum` is set to `0` in case `fixed_losses_relative` and `fixed_losses_absolute` are not passed and to `1` in case they are passed as times series or floats. At this time it is not possible to do an ivestment optimization of a stratified thermal energy storage without a non-zero `investment.minimum` (see this [issue](https://github.com/oemof/oemof-thermal/issues/174)) (#718) | |||
- The two optional parameters `fixed_losses_relative` and `fixed_losses_absolute` were added in `tests/inputs/mvs_config.json` (#718) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we will again and again recompile mvs_config.json
(from the tests/inputs
csv
data), so adding the parameters here will probably be lost. Is there a reason that you have this additionally to the benchmark test input files?
"csv_elements", | ||
) | ||
|
||
json = A1.create_json_from_csv( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very efficient :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, wait a second. @Bachibouzouk @MaGering introduces two parameters here that are not possible to add to KNOWS_EXTRA_PARAMETERS
(yet). They are added in A1
. However, if EPA provides us with input without those parameters, the simulation will fail. I tested that by running the mvs with the json input file without the two parameters:
13:03:01-INFO-Initializing oemof simulation.
13:03:01-INFO-Adding components to oemof energy system model...
Traceback (most recent call last):
File "mvs_tool.py", line 4, in <module>
main()
File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\cli.py", line 160, in main
dict_values, save_energy_system_graph=save_energy_system_graph
File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\D0_modelling_and_optimization.py", line 91, in run_oemof
dict_values, dict_model, model
File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\D0_modelling_and_optimization.py", line 195, in adding_assets_to_energysystem_model
model, dict_values[asset_group][asset], **dict_model
File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\D1_model_components.py", line 153, in storage
**kwargs,
File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\D1_model_components.py", line 328, in check_optimize_cap
func_optimize(model, dict_asset, **kwargs)
File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\D1_model_components.py", line 629, in storage_optimize
float(dict_asset[STORAGE_CAPACITY][losses][VALUE])
KeyError: 'fixed_thermal_losses_relative'
I am not sure if we need to make it so, that the MVS always adds the parameters after reading the json file, or if this is simlpy something that has to be added via the parser.
Remark @MaGering: Actually, adding the parameters to the json file is correct, sorry! After all, the mvs_config.json
file would be generated from the csv files with those two parameters. The issue is only with the EPA.
@MaGering - you can add the two parameters to the for example, say your extra parameter is within
|
Hi @Bachibouzouk! Thanks for helping me out with this. Can you give me a hint how I can test my adaptations? How can I go to |
I've done this in commit f827132. Both links should work as soon as this PR is merged to dev. If you click them now it will lead you to a
Just to be sure, I don't have to do anything with
You're absolutely right! I changed that with commit 5761ce2. Now there is only one of these warnings in
I'm sorry. Next time I will learn doing this! Thanks for your patience reviewing this PR and your credit above. :) |
@Bachibouzouk I provided the adjustment in |
When #781 is merged you can then run
you can also run
in a standalone .py file |
does that work @smartie2076. I would do |
This should work. Aren't your parameter defined as |
Yes! For the relative losses... For absolute losses it should be
Without as usual going trial and error I couldn't find out what is needed in this line... 😅 |
Yes, one could then question the need of a for loop when we have only two parameters with different treatment (I think it takes less lines of code to not have the loop ... ;)) |
Yes you're right. It's late. ;) I'll kick the loop when I correct it. |
…lt value setting if not passed
…icient_capacity/...
31e268b
to
c7c36bd
Compare
Fix #667
With this PR it is possible to model the stratified thermal storage component, which has been developed in oemof.thermal, passing two further parameter
fixed_losses_relative
andfixed_losses_absolute
to the storage component.In contrast to PR #711 the two parameters are processed in the mvs via
A1_csv_to_json.py
and hence are not optional parameters anymore (see #711 (comment) and #711 (comment)).The following steps were realized, as well (if applies):
black . --exclude docs/
)EXECUTE_TESTS_ON=master pytest
)Please mark above checkboxes as following:
❌ Check not applicable to this PR
For more information on how to contribute check the CONTRIBUTING.md.